platform.h: fixed clang-analyzer-core.BitwiseShift warnings and adjusted types of *_bit members#7271
Conversation
…usted types of `*_bit` members
|
The original CSA warnings: I filed https://trac.cppcheck.net/ticket/13600 about detecting it ourselves. The redundant code could be extracted into a helper function. I will take a look at that with fixing the TODO in a follow-up. |
| << " int_bit=\"" << settings.platform.int_bit << '\"' | ||
| << " long_bit=\"" << settings.platform.long_bit << '\"' | ||
| << " long_long_bit=\"" << settings.platform.long_long_bit << '\"' | ||
| << " char_bit=\"" << static_cast<unsigned>(settings.platform.char_bit) << '\"' |
There was a problem hiding this comment.
uint8_t is treated as a character with stream insertion operators. This caused invalid dump files to be written.
First, we should not write these files raw but using TinyXML2.
Second, I wonder if it would make sense to have a check which detects the usage of uint8_t in stream insertion operators. If you want it to a be an actual character you should probably use {unsigned|signed} char or char8_t.
There was a problem hiding this comment.
Second, I wonder if it would make sense to have a check which detects the usage of uint8_t in stream insertion operators. If you want it to a be an actual character you should probably use {unsigned|signed} char or char8_t.
Spontanously that checker sounds reasonable to me.
If char_bit will always be a positive integer then that clang warning is technically a false positive. We should ensure that char_bit is not assigned 0 when a platform xml file is loaded. I have the feeling that could have bad effects in other places also. |
| } | ||
| if (bits > 0 && bits < MathLib::bigint_bits) | ||
| v.intvalue &= (((MathLib::biguint)1)<<bits) - 1; | ||
| v.intvalue &= (1ULL<<bits) - 1; |
There was a problem hiding this comment.
after your changes this would not work well the day we switch to 128-bit bigints.. however I guess there are plenty of other code that will not work perfectly that day.
There was a problem hiding this comment.
This actually causes a compilation failure when using the 128-bit int. That's how I came across this.
The type does not guarantee that.
That's why I added the asserts. That should actually be handled during the loading of the platform. The platform loading might lack most of the error handling - at least it was at some point and I did not check if I added that yet. |
2 participants
No description provided.